Add full functionality of `classnames` package to classList #88

kcirtaptrick · 2021-09-13T06:41:44Z

This allows classList to accept an array of classNames, omitting falsey values (except for 0, reasoning described in comment).

This will allow users to put optional classNames in className, and not have to worry about omitting an unset value. The following shows the common usecase of passing through a classname:

// BEFORE
function Component({ className }) {
  return <div className={`component-class-name ${className || ""}`}> // nit: this will have an extra space and removing it is much more verbose
    ...
  </div>;
}

// AFTER
function Component({ className }) {
  return <div classList={['component-class-name', className]}>
    ...
  </div>
}

This is fully backwards compatible and accepts objects & nested arrays:

function Collapsible({ className, classList }) {
  const [collapse, setCollapse] = createSignal(false);

  return <div classList={['collapsible', { collapse: collapse() }, className, classList]}>
    ...
  </div>;
}

ryansolid · 2021-09-13T20:36:03Z

I've been slow on this one (and it has come up before) mostly in that we need to apply the support in at least 4 places including client and server side compiler and runtimes. For the most part I skip using these helpers if the compiler can analyze what we need(ie.. if it is a literal in the binding we know that it's fixed if there are no spreads or no computed properties).

But unclear if this array form can apply the same compiler level optimization. Probably if we inline a conditional. Like right now I don't optimize {[className]: true} which is pretty much identical to this. It's a bit unfortunate if arrays are just de-opt in general but maybe we can do something about this.

One thing I wasn't clear about on this part of the solution was does it handle unsetting properly and with arrays do we have to consider order? Like if someone passed in [aString(), anObject()] or even a anArray() that could apply class rules differently like if the array was made new each time with arguments in different orders.

kcirtaptrick · 2021-09-14T05:37:29Z

But unclear if this array form can apply the same compiler level optimization.

Hmm I'm not familiar with how the compiler handles classList, can you please point me in the right direction?

One thing I wasn't clear about on this part of the solution was does it handle unsetting properly and with arrays do we have to consider order?

Regarding order, I'm not sure if I understand the issue, but this current solution uses existing functionality that clears and rewrites the dom classList which should work regardless of order.

ryansolid · 2021-09-14T06:00:18Z

packages/dom-expressions/src/client.js

 for (i = 0, len = prevKeys.length; i < len; i++) {
 const key = prevKeys[i];
 if (!key || key === "undefined" || key in value) continue;
 toggleClassKey(node, key, false);
 delete prev[key];
 }


This loop compares prev keys that and if they are missing from the value it unsets them. I'm gathering we need to update to support objects in arrays.

Oh mb, missed the key in value, I have a couple of solutions in mind. First I'm thinking the simplest way to achieve this is to just wipe them all and rebuild them. At first that sounds like it would have poor performance, but I think its unlikely developers will be setting enough classnames to make this significant, even on several components. Another option would be to track present classes with setClasses, and remove all other classes.
I'll draft up these 2 scenarios so you'll have some more context.

kcirtaptrick · 2021-09-15T04:54:00Z

packages/dom-expressions/src/client.js

 for (i = 0, len = prevKeys.length; i < len; i++) {
 const key = prevKeys[i];
 if (!key || key === "undefined" || key in value) continue;
 toggleClassKey(node, key, false);
 delete prev[key];
 }


Scenario 1, wipe all classes. These will be rebuilt in setClasses

Suggested change

for (i = 0, len = prevKeys.length; i < len; i++) {

const key = prevKeys[i];

if (!key || key === "undefined" || key in value) continue;

toggleClassKey(node, key, false);

delete prev[key];

}

for (const key of Object.keys(prev)) {

toggleClassKey(node, key, false);

delete prev[key];

}

kcirtaptrick · 2021-09-15T04:58:56Z

packages/dom-expressions/src/client.js

+ (function setClasses(classes) {
+ if (classes instanceof Array) {
+ for (const key of classes) {
+ // classList setting classes for all numbers except for 0 may be
+ // unexpected behavior, you must explicity omit 0 using `n || false`
+ if (!key && key !== 0) continue;
+ if (typeof key === 'object') { // Both objects and arrays
+ setClasses(key);
+ continue;
+ }
+ toggleClassKey(node, key, true);
+ prev[key] = true;
+ }
+ } else if (typeof classes === 'object') {
+ for (const [key, enabled] of Object.entries(classes)) {
+ if (!enabled) continue;
+ toggleClassKey(node, key, true);
+ prev[key] = true
+ }
+ }
+ })(value);
+


Scenario 2, track classes & remove absent

Suggested change

(function setClasses(classes) {

if (classes instanceof Array) {

for (const key of classes) {

// classList setting classes for all numbers except for 0 may be

// unexpected behavior, you must explicity omit 0 using `n || false`

if (!key && key !== 0) continue;

if (typeof key === 'object') { // Both objects and arrays

setClasses(key);

continue;

}

toggleClassKey(node, key, true);

prev[key] = true;

}

} else if (typeof classes === 'object') {

for (const [key, enabled] of Object.entries(classes)) {

if (!enabled) continue;

toggleClassKey(node, key, true);

prev[key] = true

}

}

})(value);

const present = new Set();

(function setClasses(classes) {

if (classes instanceof Array) {

for (const key of classes) {

// classList setting classes for all numbers except for 0 may be

// unexpected behavior, you must explicity omit 0 using `n || false`

if (!key && key !== 0) continue;

if (typeof key === 'object') { // Both objects and arrays

setClasses(key);

continue;

}

toggleClassKey(node, key, true);

prev[key] = true;

present.add(key);

}

} else if (typeof classes === 'object') {

for (const [key, enabled] of Object.entries(classes)) {

if (!enabled) continue;

toggleClassKey(node, key, true);

prev[key] = true

present.add(key);

}

}

})(value);

for(const key of Object.keys(prev)) {

if(!present.has(key)) {

toggleClassKey(node, key, false);

delete prev[key];

}

}

also delete this snippet

for (i = 0, len = classKeys.length; i < len; i++) { const key = classKeys[i], classValue = !!value[key]; if (!key || key === "undefined" || prev[key] === classValue) continue; toggleClassKey(node, key, classValue); prev[key] = classValue; }

kcirtaptrick · 2021-09-15T05:00:59Z

Now that I write these 2 scenarios down, the first one seems to me like the way to go, I don't imagine the second will have any significant performance benefit, but it does add plenty of complexity

ANFADEV · 2022-06-27T08:37:12Z

Hi, are there any updates on this?

iongion · 2023-06-01T08:09:59Z

Recently discovered solid and I am toying around with porting an UI framework https://stackblitz.com/edit/solidjs-templates-gfcotu

Solid has classList - which is cool, but these are my findings

Although name says list, it is actually a dictionary
I can't ensure order, I don't know in what order the classes will be added to the element based on the keys of the object, this is a no go, especially when integrating with 3rd parties where I cannot control order of css
Limited expressivity

classnames npm package is 100 % perfect for this

      class={classNames(
        Classes.COLLAPSE,
        {
          [Classes.COLLAPSE_OPEN]: props.isOpen,
          [Classes.COLLAPSE_CLOSE]: !props.isOpen,
        },
        props.class
      )}

if props.class is undefined, how will that look like with classList version ?

    classList={{
      [Classes.COLLAPSE]: true,
      [Classes.COLLAPSE_OPEN]: props.isOpen,
      [Classes.COLLAPSE_CLOSE]: !props.isOpen,
      [props.class || ""]: true
    }}

Would really appreciate this PR being given considerations, other discussions

kcirtaptrick force-pushed the main branch from 5bcfbfa to a101a48 Compare September 13, 2021 06:44

Add full functionality of classnames package to classList

7c6f5ba

kcirtaptrick force-pushed the main branch from a101a48 to 7c6f5ba Compare September 13, 2021 06:46

ryansolid reviewed Sep 14, 2021

View reviewed changes

kcirtaptrick commented Sep 15, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add full functionality of `classnames` package to classList #88

Add full functionality of `classnames` package to classList #88

kcirtaptrick commented Sep 13, 2021 •

edited

Loading

ryansolid commented Sep 13, 2021

kcirtaptrick commented Sep 14, 2021

ryansolid Sep 14, 2021

kcirtaptrick Sep 15, 2021 •

edited

Loading

kcirtaptrick Sep 15, 2021

kcirtaptrick Sep 15, 2021 •

edited

Loading

kcirtaptrick commented Sep 15, 2021

ANFADEV commented Jun 27, 2022

iongion commented Jun 1, 2023

Add full functionality of classnames package to classList #88

Are you sure you want to change the base?

Add full functionality of classnames package to classList #88

Conversation

kcirtaptrick commented Sep 13, 2021 • edited Loading

ryansolid commented Sep 13, 2021

kcirtaptrick commented Sep 14, 2021

ryansolid Sep 14, 2021

Choose a reason for hiding this comment

kcirtaptrick Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

kcirtaptrick Sep 15, 2021

Choose a reason for hiding this comment

kcirtaptrick Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

kcirtaptrick commented Sep 15, 2021

ANFADEV commented Jun 27, 2022

iongion commented Jun 1, 2023

Add full functionality of `classnames` package to classList #88

Add full functionality of `classnames` package to classList #88

kcirtaptrick commented Sep 13, 2021 •

edited

Loading

kcirtaptrick Sep 15, 2021 •

edited

Loading

kcirtaptrick Sep 15, 2021 •

edited

Loading